try to make rustc-dev work with and without LTO - #161535
Conversation
|
@bors try jobs=dist*linux |
try to make rustc-dev work with and without LTO try-job: dist*linux
This comment has been minimized.
This comment has been minimized.
TIL glob is supported. |
| // Compiling C deps, like jemalloc and llvm-wrapper, should be with | ||
| // the same LTO mode as the Rust code they are linked into. | ||
| // Compiling C deps, like jemalloc and llvm-wrapper, should be with the same LTO mode as | ||
| // the Rust code they are linked into. We don't pass the checks cc-rs uses to |
There was a problem hiding this comment.
should be with the same LTO mode as the Rust code they are linked into
Just a note that this doesn't seem very true.
There was a problem hiding this comment.
FWIW, the discussion happens in #t-compiler > Analysing broken jemalloc rlib
There was a problem hiding this comment.
Just a note that this doesn't seem very true.
I just reformatted that comment, I don't know if it is correct.
What would be more correct?
|
Should we do something like this after the if lto_cflag.is_some()
&& builder.config.bootstrap_override_lld.is_used()
&& !target.is_msvc()
{
self.rustflags.arg("-Clink-args=-Wl,--fat-lto-objects");
}I am testing dist build on my machine still though. |
|
I guess we want a perf build @rust-timer build a4edfb9 |
This comment has been minimized.
This comment has been minimized.
I have no idea. |
This comment was marked as outdated.
This comment was marked as outdated.
|
This did fix Miri but I guess it also broke LTO. I'll try the magic linker incantation you mentioned above then. |
8975695 to
d2077da
Compare
|
@bors try jobs=dist*linux |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
try to make rustc-dev work with and without LTO try-job: dist*linux
| // Make sure the linker actually uses the fat LTO part. | ||
| if lto_cflag.is_some() | ||
| && builder.config.bootstrap_override_lld.is_used() | ||
| && !target.is_msvc() |
There was a problem hiding this comment.
FWIW I don't understand why we need to check for MSVC here, given that above we already check for clang.
Also, when does it ever make sense to set -ffat-lto-objects without setting the link-args as well?
There was a problem hiding this comment.
IIRC clang-cl.exe still produces COFF object, right? So even it is clang-like we should not pass that link arg, as it is not supported yet.
https://llvm.org/docs/FatLTO.html#supported-file-formats
Or alternativelu we should just apply to x86_64 Linux as we previously discussed.
There was a problem hiding this comment.
https://releases.llvm.org/22.1.0/tools/lld/docs/ReleaseNotes.html#coff-improvements
Oh! In LLVM 22 COFF gains the support of fat LTO.
Though according to bjorn3 we are still on 21 right now.
There was a problem hiding this comment.
Also, when does it ever make sense to set -ffat-lto-objects without setting the link-args as well?
And yeah it probably makes little sense. We may be able to just figure out the minimal condition and coerce them in one of block
There was a problem hiding this comment.
Maybe the entire code can be simplified to this?
let lto_cflag = if matches!(self.mode, Mode::Rustc | Mode::ToolRustcPrivate)
&& is_lto_stage(&self.compiler)
&& builder.cc_tool(target).is_like_clang()
&& builder.config.bootstrap_override_lld.is_used() // unsure if we should take care of non-dist config for downstream packagers
{
let lto_cflag = match builder.config.rust_lto {
RustcLto::Thin => Some("-flto=thin -ffat-lto-objects"),
RustcLto::Fat => Some("-flto=full -ffat-lto-objects"),
RustcLto::ThinLocal | RustcLto::Off => None,
};
if lto_cflag.is_some() {
self.rustflags.arg("-Clink-args=-Wl,--fat-lto-objects");
}
lto_cflag
} else {
None
};There was a problem hiding this comment.
I copied that, and it seems to work. 🤷
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5db500f): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (secondary -8.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 468.574s -> 466.803s (-0.38%) |
|
On aarch64-linux, the rustc-dev component went from 125 MiB to 128 MiB. That seems fine? That's compressed size obviously though, so this may consume a lot more disk space if it compresses very well. If I check locally the whole folder |
|
Yeah I think this should be ready now. I don't actually understand why the conditions are how they are, I just copied that from @weihanglo. ;) I kicked off another perf run to be sure but that last reorganization shouldn't change anything. r? @Kobzol |
|
|
|
Finished benchmarking commit (a259cfd): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.1%, secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 469.531s -> 468.171s (-0.29%) |
| && is_lto_stage(&self.compiler) | ||
| && builder.cc_tool(target).is_like_clang() | ||
| // unsure if we should take care of non-dist config for downstream packagers | ||
| && builder.config.bootstrap_override_lld.is_used() |
There was a problem hiding this comment.
Hmm, this config is only used for speeding up the build process itself, and it should in fact be completely unnecessary on x64 now, to the point where we should remove it from its CI config.
Why is this condition needed? Isn't the whole point of the config below that the resulting code will be compilable even without LLD?
There was a problem hiding this comment.
I'm totally fine with removing it, all these conditions are a mystery to me. ;)
60ea0f4 to
85cc84f
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
try to make rustc-dev work with and without LTO
| }; | ||
| // Make sure the linker actually uses the fat LTO part. | ||
| if lto_cflag.is_some() { | ||
| self.rustflags.arg("-Clink-args=-Wl,--fat-lto-objects"); |
There was a problem hiding this comment.
IIUC, this flag is needed to make the linker even recognize the bitcode in fat LTO objects. It is strange that this is not the default (unlike LTO for "thin" objects which apparently is the default). Does this mean we're also not LTO'ing jemalloc for the distributed Miri, Clippy etc any more?
There was a problem hiding this comment.
I'll do a Clippy benchmark after the current one finishes to check.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0ff57e8): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary 7.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary -0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 469.244s -> 471.258s (0.43%) |
|
@bors try jobs=dist-x86_64-linux profiles=clippy |
This comment was marked as outdated.
This comment was marked as outdated.
|
@bors try jobs=dist-x86_64-linux @rust-timer queue profiles=clippy |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
This comment has been minimized.
This comment has been minimized.
try to make rustc-dev work with and without LTO try-job: dist-x86_64-linux
|
Queued 85c799e with parent 04a3cad, future comparison URL. |
View all comments
This might fix the issues introduced by #161260.